Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI | add :types to query_option so dialyzer is happy #234

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

addUsername
Copy link
Contributor

First, thank you for your awesome library!!

I've just update it to last version and everything works except the CI, dialyzer fails because :types (opt needed when RowBinary) is not included in query_option

Before this commit dialyzer fails with:

The function call will not succeed.

Ch.query(_pid :: any(), _query :: <<_::8, _::size(1)>>, _events :: [map()], [
  {:types, [binary()]},
  ...
])

will never return since the success typing is:
(
  atom()
  | pid()
  | {atom(), _}
  | {:via, atom(), _}
  | %DBConnection{:conn_mode => _, :conn_ref => reference(), :pool_ref => _},
  binary()
  | maybe_improper_list(
      binary() | maybe_improper_list(any(), binary() | []) | byte(),
      binary() | []
    ),
  any(),
  [
    {:command
     | :database
     | :deadline
     | :decode
     | :encode
     | :format
     | :headers
     | :log
     | :password
     | :queue
     | :settings
     | :timeout
     | :username,
     atom() | binary() | (map() -> any()) | [{_, _}] | integer() | {atom(), atom(), [any()]}}
  ]
) :: {:error, %{:__exception__ => true, :__struct__ => atom(), atom() => _}} | {:ok, _}

and the contract is
(DBConnection.conn(), iodata(), params, [query_option()]) ::
  {:ok, Ch.Result.t()} | {:error, Exception.t()}
when params: map() | [term()] | [row :: [term()]] | iodata() | Enumerable.t()

now it is happy:

$ mix dialyzer
Finding suitable PLTs
Checking PLT...
[ :ch,  ...]
Looking up modules in dialyxir_erlang-26.0.2_elixir-1.15.4_deps-dev.plt
Finding applications for dialyxir_erlang-26.0.2_elixir-1.15.4_deps-dev.plt
Finding modules for dialyxir_erlang-26.0.2_elixir-1.15.4_deps-dev.plt
Checking 463 modules in dialyxir_erlang-26.0.2_elixir-1.15.4_deps-dev.plt
Adding 2013 modules to dialyxir_erlang-26.0.2_elixir-1.15.4_deps-dev.plt
done in 0m55.57s
No :ignore_warnings opt specified in mix.exs and default does not exist.

Starting Dialyzer
[
  check_plt: false,
  init_plt: ~c"/app/_build/dev/dialyxir_erlang-26.0.2_elixir-1.15.4_deps-dev.plt",
  files: [ ...],
  warnings: [:unknown]
]
Total errors: 0, Skipped: 0, Unnecessary Skips: 0
done in 0m5.36s
done (passed successfully)

I hope i didn't mess up somewhere 😄

lib/ch.ex Outdated
@@ -55,6 +55,7 @@ defmodule Ch do
| {:command, Ch.Query.command()}
| {:headers, [{String.t(), String.t()}]}
| {:format, String.t()}
| {:types, [String.t()]}
Copy link
Contributor

@ruslandoga ruslandoga Jan 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋

Suggested change
| {:types, [String.t()]}
| {:types, [String.t() | atom | tuple]}

Or maybe we can add something like Ch.Types.t or Ch.RowBinary.type and use it here.

  @type t ::
          :boolean
          | :string | {:fixed_string, size :: integer}
          | :u8 | :u16 | :u32 | :u64 | :u128 | :u256
          | :i8 | :i16 | :i32 | :i64 | :i128 | :i256
          | :f32 | :f64
          | :date | :date32
          | :datetime | {:datetime, timezone :: String.t()} | {:datetime64, precision :: integer, timezone :: String.t()}
          | :uuid
          | :ipv4 | :ipv6
          | :point | :ring | :polygon | :multipolygon
          | :nothing
          | {:array, t}
          | {:tuple, [t]}
          | {:map, t, t}
          | {:nullable, t}
          | {:low_cardinality, t}
          | {:simple_aggregate_function, name :: String.t(), t}
          | {:decimal, precision :: integer, scale :: integer} | {:decimal32, scale :: integer} | {:decimal64, scale :: integer} | {:decimal128, scale :: integer} | {:decimal256, scale :: integer}
          | {:enum8, mapping :: [{name :: String.t(), value :: integer}]} | {:enum16, mapping :: [{name :: String.t(), value :: integer}]}

and then

@type query_option ::
  # ...
  | {:types, [String.t() | Ch.Types.t()]}
  # ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, second option looks nicer, but I'm not sure on how to implement it.. whatever works for you 😄

Co-authored-by: ruslandoga <[email protected]>
@ruslandoga
Copy link
Contributor

@addUsername thank you!

@ruslandoga ruslandoga merged commit 57493ec into plausible:master Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants